Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Address webhook custom confirmations and confidence #23

Merged
merged 2 commits into from
Dec 24, 2015

Conversation

lukewarm
Copy link
Contributor

Added two parameters to subscribe_to_address_webhook that allows custom values for confidence and confirmations.

Also fixed a bug in get_addresses_details .

Added to function subscribe_to_address_webhook two parameters that accept custom values of confirmations and confidence for tx-confirmation and tx-confidence event types, respectively.
@lukewarm lukewarm changed the title Address webhook customconfirmation and confidence Address webhook custom confirmations and confidence Dec 13, 2015
@mflaxman
Copy link

Thanks @lukewarm and this LGTM!

Would you mind adding test coverage? I'm requiring this for all new PRs. If
not, I'm happy to add. Yours is a unique case in that testing webhooks
requires a server that can accept inbound connections (ngrok would work
great here) and doing a basic spend TX using BCY (see existing tests).

If this is too much of a pain I'm happy to add it later this week.

Thanks again for the PR.
On Dec 13, 2015 1:11 PM, "lukewarm" notifications@github.com wrote:

Added two parameters to subscribe_to_address_webhook that allows custom
values for confidence and confirmations.

Also fixed a bug in get_addresses_details .

You can view, comment on, or merge this pull request online at:

#23
Commit Summary

  • Address webhook desired confirmation and confidence
  • Fix bug in api.get_addresses_details

File Changes

Patch Links:


Reply to this email directly or view it on GitHub
#23.

@lukewarm
Copy link
Contributor Author

I very much want to contribute by writing the test, but I don't think I know how to do it even with your pointers...

@mflaxman mflaxman merged commit c30a81b into blockcypher:master Dec 24, 2015
@mflaxman
Copy link

Thanks again @lukewarm and sorry for the delay in getting this merged. I also added test coverage so that the bugfix you submitted can't happen again, good catch.

The idea I had of using ngrok so that this test could be run on any machine was not actually a great one since you'd still need an HTTP server to handle the inbound request, and you'd also need to make the result of that available to the unit test. I think the better way to do it would be something like if requestb.in had an API so that you could write a test as follows:

  1. create a new requestb.in programatically
  2. subscribe to a blockcypher webhook via API
  3. sign a transaction to the address you subscribed to
  4. query requestb.in to confirm that the webhook was sent and is valid

It looks like requestb.in's paid product (called runscope) has a free version that might offer this feature, but I spent 10 minutes playing around with it and couldn't see if it supports that.

I'm opening a separate issue to see if there's a way to do something like this, as I'd like webhook subscriptions should have full test coverage in this library.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants